Skip to content

src: validate stdio entries in process_wrap#61978

Open
dhruv7539 wants to merge 3 commits intonodejs:mainfrom
dhruv7539:codex/fix-process-wrap-stdio-entries
Open

src: validate stdio entries in process_wrap#61978
dhruv7539 wants to merge 3 commits intonodejs:mainfrom
dhruv7539:codex/fix-process-wrap-stdio-entries

Conversation

@dhruv7539
Copy link

This guards ProcessWrap::ParseStdioOptions() against non-object entries in options.stdio.

Before this change, each options.stdio[i] value was cast directly with val.As(). If Array.prototype is polluted (for example with a setter at index 2), child_process.spawn() can hit a fatal abort (FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal) instead of throwing a JS error.

This patch:

  • validates each options.stdio[i] entry is an object and throws ERR_INVALID_ARG_TYPE otherwise
  • adds a regression test (test/parallel/test-child-process-array-prototype-setter.js) that reproduces the prototype-pollution scenario in a subprocess and verifies Node does not abort

Refs: #56531

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Feb 25, 2026
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 25, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2026
@nodejs-github-bot
Copy link
Collaborator

@dhruv7539
Copy link
Author

Follow-up fix pushed in 9aa1a1d: the new test regex was over-escaped (matching literal backslashes), which caused the CI failure.

Could a collaborator please add the request-ci label to retrigger Jenkins for this commit?

@dhruv7539 dhruv7539 requested a review from addaleax February 25, 2026 18:25
@addaleax addaleax added request-ci Add this label to start a Jenkins CI on a PR. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 25, 2026
@addaleax
Copy link
Member

@dhruv7539 The linter is failing here anyway, that should be addressed first

@codecov
Copy link

codecov bot commented Feb 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.75%. Comparing base (2ebe496) to head (9aa1a1d).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61978      +/-   ##
==========================================
- Coverage   89.77%   89.75%   -0.02%     
==========================================
  Files         674      674              
  Lines      205670   205709      +39     
  Branches    39426    39442      +16     
==========================================
+ Hits       184635   184641       +6     
- Misses      13272    13316      +44     
+ Partials     7763     7752      -11     
Files with missing lines Coverage Δ
src/process_wrap.cc 70.96% <100.00%> (+1.29%) ⬆️

... and 35 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dhruv7539
Copy link
Author

Pushed follow-up commit bdb66fb to fix the clang-format failure in src/process_wrap.cc.

Could a collaborator please add the request-ci label so Jenkins runs on this latest commit?

@dhruv7539 dhruv7539 requested a review from addaleax February 25, 2026 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants